Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark the FunctionCall/ResultContent.Exception properties as [JsonIgnore] #5492

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Oct 9, 2024

Given implementation details of the JSON source generator today, even with the converter applied to these properties, code is still being generated for Exception, leading to unsuppressable trimmer warnings.

Closes #5483

Microsoft Reviewers: Open in CodeFlow

Given implementation details of the JSON source generator today, even with the converter applied to these properties, code is still being generated for Exception, leading to unsuppressable trimmer warnings.
/// information to an AI service, a human-readable representation of those conditions should be supplied.
/// </param>
/// <param name="exception">Any exception that occurred when invoking the function.</param>
public FunctionResultContent(string callId, string name, object? result, Exception? exception)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to expose a ctor overload accepting Exception given that the property is settable?

Copy link
Member Author

@stephentoub stephentoub Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to avoid breaking binaru changes right now if they're not necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but if we can't make them now it will be even more difficult to make them later.

Copy link
Member Author

@stephentoub stephentoub Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather take any such changes in batch when we have a meaningful set, do a more thorough review based on received feedback, etc. Force people to rev fewer times.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we remember/have enough context to remove this constructor when such a review is going to be made? Unless we release new bits to customers on a very frequent basis I don't see how postponing such a decision would impact churn substantially.

Copy link
Member Author

@stephentoub stephentoub Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each one of these is a binary breaking change. If every single release makes such a change but is binary breaking, it effectively means that the ecosystem gets reset with every single release. At a minimum these are shipping monthly now along with the rest of dotnet/extensions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, but at the same time this seems fair game or even expected when evolving previews.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this constructor is not a meaningful fix. It doesn't improve what someone is able to do. All it does is force someone to use a different syntax to construct it. Why would we want to reset the ecosystem for such a change? When there are other binary breaking changes we decide to make in preview, then we can make this at the same time, but I don't see the value in just making this one. And shipping will be frequent enough that it very well could be the only one.

@stephentoub stephentoub merged commit 3970109 into dotnet:main Oct 9, 2024
6 checks passed
@stephentoub stephentoub deleted the jsonignoreexception branch October 9, 2024 20:13
@github-actions github-actions bot locked and limited conversation to collaborators Nov 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Address trimmer warning on Microsoft.Extensions.AI due to JSON source generator
3 participants